[Clang][AST] Fix MS Mangle concept uneval context template instantiation crash #117845

MaxEW707 · 2024-11-27T04:21:25Z

MSVC mangling got inadvertently broken here, #83997, when it was fixed what decl context a lambda is apart of for uneval contexts.

https://godbolt.org/z/K6jb5v145 for reference.

Given the following code snippet

template <typename T>
concept C = requires(const T& t)
{
    { T::test([](){}) };
};

template<typename T, typename = void>
struct Widget;

template <C T>
struct Widget<T> {};

struct Baz
{
    template<typename F>
    static constexpr decltype(auto) test(F&& f) {}
};

void test()
{
    Widget<Baz> w;
}

Baz::test has a deduced return type which means we must instantiate that template even in an unevaluated context.
The lambda inside the concept is within the decl context of struct Widget<T> {};. So we end up needing to mangle a name of Baz::test<Widget<template-type-0-0>::lambda()>>() since the lambda isn't apart of an instantiated substituted class Widget yet at the point the lambda is instantiated.

Upon template instantation of test we end up asking for the mangled name so we can add this instantiation to CodeGenModule::DefferredDecls since test is now referenced but not yet used.

I think the longer term more correct solution is to key DefferedDecls off of something else than the mangled name to avoid having to mangle names for instantations that are referenced but will never be used since they are only instantiated from an unevaluated context.

As a fix for the regression I just created a custom mangling scheme for this case since MSVC has no comparable naming scheme as such a template will never be emitted into the resulting obj as it will never be used.

Similarly the following case is also broken for our msvc compat, https://godbolt.org/z/evPo6KTfE, since we do not mangle decltype(Expr). The Itanium ABI gets away with this as the entire decltype(Expr) expression must be mangled as is for that ABI.The MSVC ABI never mangles a decltype. Only the final canonical deduced type is mangled into the name so we never need to worry about mangling decltype except in the uneval context edge case.
I want to hold off fixing this case until I can see if I can come up with a better more holistic solution around DefferedDecls first. I'll get a github tracking issue created for it however.

If anyone has a better alternative to solve this let me know. I couldn't come up with one since the template instantation code path for uneval contexts takes the same code path as any template instantation and thus requires the ability to get back a mangled name for said template instantation.

llvmbot · 2024-11-27T04:22:01Z

@llvm/pr-subscribers-clang

Author: Max Winkler (MaxEW707)

Changes

Fixes #115990.

MSVC mangling got inadvertently broken here, #83997, when it was fixed what decl context a lambda is apart of for uneval contexts.

https://godbolt.org/z/K6jb5v145 for reference.

Given the following code snippet

template &lt;typename T&gt;
concept C = requires(const T&amp; t)
{
    { T::test2([](){}) };
};

template&lt;typename T, typename = void&gt;
struct Widget;

template &lt;C T&gt;
struct Widget&lt;T&gt; {};

struct Baz
{
    template&lt;typename F&gt;
    static constexpr decltype(auto) test2(F&amp;&amp; f) {}
};

void test()
{
    Widget&lt;Baz&gt; w;
}

Baz::Test has a deduced return type which means we must instantiate that template even in an unevaluated context.
The lambda inside the concept is within the decl context of struct Widget<T> {};. So we end up needing to mangle a name of Baz::test2<Widget<template-type-0-0>::lambda()>>() since the lambda isn't apart of an instantiated substituted class Widget yet at the point the lambda is instantiated.

Upon template instantation of test2 we end up asking for the mangled name so we can add this instantiation to CodeGenModule::DefferredDecls since test2 is now referenced but not yet used.

I think the longer term more correct solution is to key DefferedDecls off of something else than the mangled name to avoid having to mangle names for instantations that are referenced but will never be used since they are only instantiated from an unevaluated context.

As a fix for the regression I just created a custom mangling scheme for this case since MSVC has no comparable naming scheme as such a template will never be emitted into the resulting obj as it will never be used.

Similarly the following case is also broken for our msvc compat, https://godbolt.org/z/evPo6KTfE, since we do not mangle decltype(Expr). The Itanium ABI gets away with this as the entire decltype(Expr) expression must be mangled as is for that ABI.The MSVC ABI never mangles a decltype. Only the final canonical deduced type is mangled into the name so we never need to worry about mangling decltype except in the uneval context edge case.

If anyone has a better alternative to solve this let me know. I couldn't come up with one since the template instantation code path for uneval contexts takes the same code path as any template instantation and thus requires the ability to get back a mangled name for said template instantation.

Full diff: https://github.com/llvm/llvm-project/pull/117845.diff

2 Files Affected:

(modified) clang/lib/AST/MicrosoftMangle.cpp (+9-1)
(added) clang/test/AST/ms-uneval-context-crash.cpp (+25)

diff --git a/clang/lib/AST/MicrosoftMangle.cpp b/clang/lib/AST/MicrosoftMangle.cpp
index dbc161347025c0..c177e3b1d01ddf 100644
--- a/clang/lib/AST/MicrosoftMangle.cpp
+++ b/clang/lib/AST/MicrosoftMangle.cpp
@@ -3374,7 +3374,15 @@ void MicrosoftCXXNameMangler::mangleType(const MemberPointerType *T,
 
 void MicrosoftCXXNameMangler::mangleType(const TemplateTypeParmType *T,
                                          Qualifiers, SourceRange Range) {
-  Error(Range.getBegin(), "template type parameter type") << Range;
+  Out << '?';
+
+  llvm::SmallString<64> Name;
+  Name += "<TTPT_";
+  Name += llvm::utostr(T->getDepth());
+  Name += "_";
+  Name += llvm::utostr(T->getIndex());
+  Name += ">";
+  mangleSourceName(Name);
 }
 
 void MicrosoftCXXNameMangler::mangleType(const SubstTemplateTypeParmPackType *T,
diff --git a/clang/test/AST/ms-uneval-context-crash.cpp b/clang/test/AST/ms-uneval-context-crash.cpp
new file mode 100644
index 00000000000000..b2f7e58381da81
--- /dev/null
+++ b/clang/test/AST/ms-uneval-context-crash.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fms-compatibility-version=19.33 -emit-llvm %s -o - -triple=x86_64-windows-msvc | FileCheck %s
+
+template <typename T>
+concept C = requires
+{
+    { T::test([](){}) };
+};
+
+template<typename T>
+struct Widget {};
+
+template <C T>
+struct Widget<T> {};
+
+struct Baz
+{
+    template<typename F>
+    static constexpr decltype(auto) test(F&&) {}
+};
+
+void test()
+{
+    Widget<Baz> w;
+}
+// CHECK: @"?test@@YAXXZ"

zmodem · 2024-11-27T12:39:09Z

clang/test/AST/ms-uneval-context-crash.cpp

@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fms-compatibility-version=19.33 -emit-llvm %s -o - -triple=x86_64-windows-msvc | FileCheck %s


I think doing codegen (-emit-llvm) in clang/test/AST/ is unusual, those tests normally just look at the AST (though there are exceptions). Maybe there is a better place for this?

zmodem · 2024-11-27T12:42:06Z

clang/test/AST/ms-uneval-context-crash.cpp

+{
+    Widget<Baz> w;
+}
+// CHECK: @"?test@@YAXXZ"


I suppose the mangling you're adding doesn't actually show up anywhere since it's only used for a concept? But then why do we need to mangle it in the first place?

Once we get to CodeGen, the method looks like a regular method, except for the fact that one of the template arguments is a class defined inside a concept instantiation. And CodeGen doesn't have any way to check for "is a template argument a class defined inside a concept". So, like all other methods, we stick it into DeferredDecls. And the key for the DeferredDecls map is the mangled name.

We end up trying to mangle a TemplateTypeParmType because we stick the instantiated class into the scope of an uninstantiated class template.

So no, we don't actually need to mangle it, but there's currently no way for CodeGen to check for this situation.

zmodem

The fix LGTM. I do wonder if the test should live in CodeGen/ instead, but I don't have a strong feeling about this (AST/ already seems to have some codegen tests).

cor3ntin · 2024-11-28T13:21:31Z

The fix LGTM. I do wonder if the test should live in CodeGen/ instead, but I don't have a strong feeling about this (AST/ already seems to have some codegen tests).

Moving the test would be more consistent indeed

Fix MS Mangle concept uneval context template instantiation crash

58a2620

MaxEW707 requested review from zmodem and efriedma-quic November 27, 2024 04:21

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 27, 2024

zmodem reviewed Nov 27, 2024

View reviewed changes

zmodem approved these changes Nov 28, 2024

View reviewed changes

Move to CodeGenCXX

daf0610

MaxEW707 mentioned this pull request Dec 2, 2024

[clang-cl] Cannot mangle decltype when used in an unevaluated context #118241

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang][AST] Fix MS Mangle concept uneval context template instantiation crash #117845

[Clang][AST] Fix MS Mangle concept uneval context template instantiation crash #117845

MaxEW707 commented Nov 27, 2024 •

edited

Loading

llvmbot commented Nov 27, 2024

zmodem Nov 27, 2024

zmodem Nov 27, 2024

efriedma-quic Nov 28, 2024

zmodem left a comment

cor3ntin commented Nov 28, 2024

		@@ -0,0 +1,25 @@
		// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fms-compatibility-version=19.33 -emit-llvm %s -o - -triple=x86_64-windows-msvc \| FileCheck %s

[Clang][AST] Fix MS Mangle concept uneval context template instantiation crash #117845

Are you sure you want to change the base?

[Clang][AST] Fix MS Mangle concept uneval context template instantiation crash #117845

Conversation

MaxEW707 commented Nov 27, 2024 • edited Loading

llvmbot commented Nov 27, 2024

zmodem Nov 27, 2024

Choose a reason for hiding this comment

zmodem Nov 27, 2024

Choose a reason for hiding this comment

efriedma-quic Nov 28, 2024

Choose a reason for hiding this comment

zmodem left a comment

Choose a reason for hiding this comment

cor3ntin commented Nov 28, 2024

MaxEW707 commented Nov 27, 2024 •

edited

Loading